Update entity saved states to be selected by default and partitioned by type. #21159
Conversation
|
Size Change: +21 kB (2%) Total Size: 884 kB
ℹ️ View Unchanged
|
Addison-Stavlo
changed the title
|
Actually, the more I look at the code, the history, and the designs the more it seems to make sense to get rid of this It is only added here: gutenberg/packages/editor/src/components/post-publish-button/index.js Lines 185 to 188 in 4da1dd4 And is only used to prevent saving those parent post items here: This prop was added as part of the first exploration PR into this #18029, so maybe the initial idea was not to have the parent post savable through this component. But since design mockups all seem to show this functionality, we should just remove this and allow saving of the parent post in the modal when applicable. @youknowriad can you think of any reasons why we would need to keep this prop? Currently parent posts show up in the modal but are only giving the illusion of being saved through this. We should probably just remove this and have them show up and save as expected (as opposed to my initial direction of conforming other things to this prop that doesn't seem to make sense anymore). I updated the PR to go this route as it seems to make the most sense. |
|
I agree with the reasoning here 👍 I'm not really sure why we needed this prop in the first place. Do you think it's possible to add an e2e test about this feature (multi-entity save)? |
There was a problem hiding this comment.
Do you think it's possible to add an e2e test about this feature (multi-entity save)?
Sure thing! I updated this to have an e2e test at packages/e2e-tests/specs/editor/various/multi-entity-saving.test.js. These tests walk through both the post editor as well as the site editor.
| const [ slug, _setSlug ] = useState(); | ||
| const [ theme, setTheme ] = useState(); | ||
| const [ slug, _setSlug ] = useState( '' ); | ||
| const [ theme, setTheme ] = useState( '' ); |
There was a problem hiding this comment.
It was necessary to edit these to stop a react warning from breaking the e2e tests. Without initializing as an empty string, react would complain about turning an uncontrolled component into a controlled component once the input was used.
| @@ -53,12 +53,13 @@ export default function SaveButton() { | |||
| const disabled = ! isDirty || isSaving; | |||
|
|
|||
| const [ isOpen, setIsOpen ] = useState( false ); | |||
| const open = useCallback( setIsOpen.bind( null, true ), [] ); | |||
| const close = useCallback( setIsOpen.bind( null, false ), [] ); | |||
| const open = useCallback( () => setIsOpen( true ), [] ); | |||
There was a problem hiding this comment.
These were also necessary to edit due to reacts warnings breaking e2e tests. Without this change, react would throw a warning when the button was used about useState update functions not supporting the second callback argument.
This should probably be moved to the "experimental" specs folder. That's how we distinguish the tests that can run in Core from the others (experimental ones) |
|
I'd love to get this one in as it'd improve the quality of the upcoming WP Block Talk demo :P |
That makes sense. I moved it to the experimental folder. |
|
Looks like this is not working properly for the site entity. I guess it shouldn't be a top level item. or maybe we should use the "site title" as label there. |
|
Yes, I still wish we could use the block name that triggered the entity save here, or the name of the attribute changed :) |
Thats what I would have expected to see given the functionality on master and the scope of the given changes. It shows up like that on master too:
So at least it's not a regression, but definitely something that we will need to address.
I don't understand what we mean by 'top level item' in this case, could you elaborate please?
Agreed, that would be much less ambiguous that the 'untitled' site. I'd be happy to iterate further on these things but I'd vote to address them in follow-up PR(s) if possible. |
|
@Addison-Stavlo I think the difference is between showing "Site" as a heading and "Untitled" as the checked item vs just showing the checkbox with "Site" next to it. |
Ah, ok I see now. Thank you! Il do something about that here shortly. On a related note about having the 'Untitled' appear now as opposed to before: The 'Untitled' started showing up as there was an 'Untitled' label fallback in the code but hidden behind other logic that would never trigger it: I assumed it was desired as the fallback label so now entities without titles should show 'untitled' as pointed out with the Site's entity. |
|
Ok, with a small edit to the
|
There was a problem hiding this comment.
So happy that I can actually save entities now! 🎉
(I rebased this on master before testing.)
I don't know if this is related to the PR, but I am noticing that the blue dots do not reflect which templates need updated. I took this screenshot immediately upon entering the page before editing it. (And it stays the same even after saving them).
Additionally, when I make a change to only a template part, it asks me to save the template part and the template (which I didn't expect, but maybe it is necessary??). When I change only the template, I only have to save the template.
Anyways, this does fix what is advertised: it correctly saves the templates. So I think it could be merged for the block talk, and other stuff could be follow ups
I believe it shouldn't ask to save the template in this case, I know it's not an easy bug though. It's related to how "controlled" InnerBlocks work. I proposed a solution at some point but It was discarded at that moment. If it's something you're interested in tackling le me know, I can explain further.
I believe this is "normal" because these templates come initially from the theme files and are not saved in the CPT. |
|
Looks like the site editor test is failing, I'm going to skip it to land this but consider checking it separately. Thanks. |
Is that in regards to the e2e test I added? Il take a look. It has been passing for me but I think I may need to find a way to make it a more controlled environment. Navigating to a given site's Site Editor isn't going to be as universally congruent as starting a new post is, so we may have to force it to use the demo template and maybe some other tricks to make it more consistent.. 🤔 |
Description
This PR offers 2 Bug Fixes:
And also 2 minor design changes as suggested in #20421 :
I believe these changes are a small step in the right direction for the future designs being suggested. However, I believe this is a good place to review and merge current changes to re-introduce working functionality.
More Info
1. Allows checkboxes to be toggled on/off again.
There was a logic error preventing the items to be selected/deselected for saving.
2. Allows parent post to actually save through this when applicable
This
EntitySavedStatescomponent receives anignoredForSaveprop as a list of entities to be ignored by its save function. However, the items shown in the selectable save list did not exclude entities which are part ofignoredForSave. This would cause the perception of these items (currently limited to parent entities in the Page, Post, etc. editors) to be saved when they actually were not.For reasons outlined in the comment below #21159 (comment), I think it is best to get rid of this prop altogether, so now the parent entities like 'Page' will save as expected when applicable.
3 & 4. All checkboxes selected by default & Entities partitioned by type:
Instead of all savable items being unselected by default, they will be checked by default and separated by entity type:
How has this been tested?
Tested on local docker setup in page, post, template, template part, and site editor instances.
Types of changes
Checklist: